Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Addendum to #2426 - Adminhtml loader #2475

Merged
merged 3 commits into from
Aug 24, 2022
Merged

Addendum to #2426 - Adminhtml loader #2475

merged 3 commits into from
Aug 24, 2022

Conversation

justinbeaty
Copy link
Contributor

Description (*)

I was working on a few things and noticed I needed a few fixes for #2426.

Issues:

  • Some places called Element.show('loading-mask'); directly. Because I hid the child elements, that broke the loader showing. I removed the style="display:none" from those child elements to fix it.
  • In any case, I removed those direct calls and introduced a new methods to show and hide the loader from anywhere. This has the benefit of making the loader show up with a timeout in other places besides AJAX calls.
  • Updated the loader html in a few other templates that I missed.

Related Pull Requests

#2426

Fixed Issues (if relevant)

Manual testing scenarios (*)

  1. Test similar tasks as in Add loading delay adminhtml #2426, such as sorting an admin grid
  2. Test Manage Categories page, try saving a category

Questions or comments

I apologize about the breakage, I somehow missed that the loader was displayed manually in places.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Component: ImportExport Relates to Mage_ImportExport Component: Newsletter Relates to Mage_Newsletter JavaScript Relates to js/* Template : admin Relates to admin template labels Aug 23, 2022
fballiano
fballiano previously approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems much better this way

@justinbeaty
Copy link
Contributor Author

justinbeaty commented Aug 23, 2022 •
edited
Loading

One sec, one more change incoming.

Edit: done, I improved the logic when showLoader might be called twice in a row.

@justinbeaty
Copy link
Contributor Author

seems much better this way

The more you look at the inline spaghetti javascript of Magento the more you want to cry 😭

@fballiano
Copy link
Contributor

The more you look at the inline spaghetti javascript of Magento the more you want to cry 😭

yes ahhahaha I'm working now on an issue that's driving me crazy and it's been there since forever!

@justinbeaty
Copy link
Contributor Author

The more you look at the inline spaghetti javascript of Magento the more you want to cry 😭

yes ahhahaha I'm working now on an issue that's driving me crazy and it's been there since forever!

Let me know if you need a second set of eyes. You can post on Discord if you want.

@fballiano
Copy link
Contributor

Let me know if you need a second set of eyes. You can post on Discord if you want.

Thanks! I think I'll be able to post a PR soon (maybe today) that will need some test ehheheh

@fballiano fballiano force-pushed the topic-pr-2426-addendum branch from c6b840c to de9f919 Compare August 23, 2022 23:20
@fballiano fballiano requested a review from addison74 August 23, 2022 23:20
@fballiano fballiano merged commit d175ad7 into OpenMage:1.9.4.x Aug 24, 2022
@addison74
Copy link
Contributor

This is a useful PR. On the same computer in two VMs with identical allocated resources (almost top processor, memory, nvme), OM installed in Windows 10 (XAMPP) from 450 ms no longer displays the loader, Debian 10 from 25 ms no longer displays the loader. The differences are notable, if we had set a default value and it could only be changed by editing, only some would have benefited from this feature.

@justinbeaty justinbeaty mentioned this pull request Aug 29, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addison74 addison74 addison74 approved these changes

@fballiano fballiano fballiano approved these changes

Assignees
No one assigned
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Component: ImportExport Relates to Mage_ImportExport Component: Newsletter Relates to Mage_Newsletter JavaScript Relates to js/* Template : admin Relates to admin template
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@justinbeaty @fballiano @addison74